Skip to content

Conversation

@pillo79
Copy link
Contributor

@pillo79 pillo79 commented Sep 3, 2024

This PR is based on #77641 (first 3 commits) and adds the test suite for that feature. Will be set as ready as soon as the parent PR is merged.

LLEXT test case definition has been improved to use struct syntax, so that tests can easily define many custom features. New test_cleanup callback has been added to check the proper execution of the routine linked in .fini_array.

Load the .preinit_array, .init_array and .fini_array sections in ELF
files. These sections are arrays of function pointers that are filled by
the compiler with the addresses of functions that need to be called at
startup or termination by the loader, such as C++ constructors and
destructors.

Signed-off-by: Luca Burelli <[email protected]>
llext_bringup() and llext_teardown() are intended to be used to call the
extension's own initialization and cleanup functions, respectively. They
are meant to be called by the developer after loading an extension and
before unloading it. The list of function pointers to be called is
obtained via the new llext_get_fn_table() syscall, so that they are
compatible with user mode.

llext_bootstrap() is intended to be used as the entry point for a thread
created to run an extension, in either user or kernel contexts. It will
call the extension's own initialization functions and then an additional
entry point in the same context (if desired). The same function can also
be called directly in the main thread, if only initialization is
required.

Signed-off-by: Luca Burelli <[email protected]>
Add documentation for the new LLEXT APIs that allow to call the
initialization and cleanup functions of an extension.

Signed-off-by: Luca Burelli <[email protected]>
This patch refactors the macro for test case definition to simplify the
generic case. By using variable macro arguments and default-0 fields,
the most common case does not require arguments at all and tests that
have special requirements can be defined with only the necessary fields.

Signed-off-by: Luca Burelli <[email protected]>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just int?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No special reason. We also need to be 64-bit inclusive now 😄 so I'll switch.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

int?

This commit renames the perm_setup callback to test_setup and provides
the extension as an additional parameter. It also adds a test_cleanup
callback that is called after each test completes.

Setup and cleanup functions are now called regardless of whether
CONFIG_USERSPACE is enabled or not.

Signed-off-by: Luca Burelli <[email protected]>
Add a test to check the proper support of ELF init arrays, using
the new llext_bootstrap function in place of llext_entry in the
LLEXT test suite.

Signed-off-by: Luca Burelli <[email protected]>
teburd
teburd previously approved these changes Sep 6, 2024
@pillo79 pillo79 force-pushed the pr-test-init-arrays branch from dc41a6e to ad53931 Compare September 6, 2024 13:03
@pillo79 pillo79 marked this pull request as ready for review September 6, 2024 13:04
@zephyrbot zephyrbot added the area: llext Linkable Loadable Extensions label Sep 6, 2024
Copy link
Contributor

@mathieuchopstm mathieuchopstm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Few nits if you need to make changes:


#include "llext_priv.h"

ssize_t z_impl_llext_get_fn_table(struct llext *ext, bool is_init, void *buf, size_t buf_size)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: enum fn_table_type table_type instead of bool is_init would be more future-proof, although I don't know if there are other tables we might have to make available.

Comment on lines +63 to +64
LOG_ERR("init function %i (%p) outside text region",
i, fn_ptrs[i]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: init function in string is not necessarily appropriate

Suggested change
LOG_ERR("init function %i (%p) outside text region",
i, fn_ptrs[i]);
LOG_ERR("%s function %i (%p) outside text region",
is_init ? "init" : "fini", i, fn_ptrs[i]);

Comment on lines +305 to +306
/* These regions are not extendable and must be
* referenced at most once in the ELF file.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should refer to sections rather than regions

Proposal:

Suggested change
/* These regions are not extendable and must be
* referenced at most once in the ELF file.
/* Each of these sections can only be present at most
* once in an ELF file. Finding a second one indicates
* the ELF is malformed and should be rejected.

@fabiobaltieri fabiobaltieri merged commit dfef264 into zephyrproject-rtos:main Sep 10, 2024
@pillo79 pillo79 deleted the pr-test-init-arrays branch October 2, 2024 08:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: llext Linkable Loadable Extensions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants